Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unify extension module hierarchies #894

Merged
merged 4 commits into from
Mar 30, 2024
Merged

Conversation

Ralith
Copy link
Collaborator

@Ralith Ralith commented Mar 26, 2024

Fixes #876.

As a bonus, this significantly reduces the amount of boilerplate required to wrap a new extension.

Copy link
Collaborator

@MarijnS95 MarijnS95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we're heading in the right direction to make Ash increasingly easy to use! Just wondering if we need to consider something similar for Entry/Instance/Device now.

Don't forget a changelog entry!

ash-examples/src/lib.rs Outdated Show resolved Hide resolved
generator/src/lib.rs Show resolved Hide resolved
ash/src/lib.rs Show resolved Hide resolved
@Ralith
Copy link
Collaborator Author

Ralith commented Mar 26, 2024

Updated changelog.

Changelog.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@MarijnS95 MarijnS95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's an amazing amount of repetitive hand-written code gone from the codebase! We should've done this much sooner, specifically these duplicated bits have been a continuous source of conflicts and compiler errors during the refactors of the past week.


This is also a direction I was hoping to head into with better generator support: at some point we can start autogenerating some of these functions, and use both heuristics and config files to allow keeping manual implementations where a generator just doesn't understand the desired API.

ash-examples/src/lib.rs Outdated Show resolved Hide resolved
ash/src/extensions/khr/create_render_pass2.rs Show resolved Hide resolved
@Ralith Ralith force-pushed the unified-exts branch 4 times, most recently from bd54abb to 851e297 Compare March 29, 2024 21:11
@Ralith
Copy link
Collaborator Author

Ralith commented Mar 29, 2024

Revised approach: all core and extension raw type definitions and constants are exported directly under ash::vk. This includes e.g. extension name constants, which are named in the upstream style, less VK_. Extensions modules are exported under ash::{khr, ext, ...}, and include high-level wrappers, *Fn tables, and NAME and SPEC_VERSION reexports. Core *Fn structs are exported directly under ash.

This standardizes ash::vk as being purely unopinionated "sys" style bindings, while everything else is exposed under ash. We accept a small risk of a name collision between a future vendor tag and any other items we want to expose at the crate level.

generator/src/lib.rs Outdated Show resolved Hide resolved
generator/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@MarijnS95 MarijnS95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of changes to separate code out of extensions.rs, but all for the better in the end!

@Ralith Ralith merged commit 3d84654 into ash-rs:master Mar 30, 2024
10 checks passed
@Ralith Ralith deleted the unified-exts branch March 30, 2024 00:37
@@ -76,6 +76,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- extensions/ext/ray_tracing_pipeline: Pass indirect SBT regions as single item reference (#829)
- Replaced `c_char` array setters with `CStr` setters (#831)
- `push_next()` functions now allow unsized `p_next` argument (#855)
- Flattened `ash::extensions` into `ash`, flattened `ash::vk::*` extension modules into `ash::vk`, and moved `*Fn` function pointer table structs from `ash::vk` into `ash` or the associated extension module (#894)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

flattened ash::vk::* extension modules into ash::vk

That seems slightly strange to mention as extension modules didn't exist (not even in ash::vk::*) in the latest release (they've only been introduced and resided there temporarily).

However, looks like we totally forgot to even mention that entire mod-refactor for extensions in #734.

Alas, nothing too bad, as I think we should write a proper actionable summary when publishing the 0.38 release.

MarijnS95 added a commit that referenced this pull request Mar 31, 2024
…nsion wrappers

In #894 we lost the ability to `Clone` the high-level wrappers, even
though the underlying `*Fp` table and the `vk::Instance` or `vk::Device`
handle themselves are `Clone`.  Restore that behaviour.
MarijnS95 added a commit that referenced this pull request Mar 31, 2024
…nsion wrappers (#899)

In #894 we lost the ability to `Clone` the high-level wrappers, even
though the underlying `*Fp` table and the `vk::Instance` or `vk::Device`
handle themselves are `Clone`.  Restore that behaviour.
MarijnS95 added a commit that referenced this pull request Mar 31, 2024
…th 3 specialized functions

Since the extension modularization in #894 this `enum ShaderInfoResult`
is no longer reachable through the crate hierarchy, making it impossible
for callers to match on its variants.

Not that this type was ideal to begin with: the returned `enum`
variant depended purely on the `info_type` parameter, leading to ugly
`unreachable!()`-like unwraps in caller code when the variant should
always be of the type that a caller requested.

To solve both issues, create 3 instances of the `get_shader_info()`
function for each of the 3 `vk::ShaderInfoTypeAMD`s that a caller can
request.
MarijnS95 added a commit that referenced this pull request Apr 1, 2024
…th 3 specialized functions (#901)

Since the extension modularization in #894 this `enum ShaderInfoResult`
is no longer reachable through the crate hierarchy, making it impossible
for callers to match on its variants.

Not that this type was ideal to begin with: the returned `enum`
variant depended purely on the `info_type` parameter, leading to ugly
`unreachable!()`-like unwraps in caller code when the variant should
always be of the type that a caller requested.

To solve both issues, create 3 instances of the `get_shader_info()`
function for each of the 3 `vk::ShaderInfoTypeAMD`s that a caller can
request.
@MarijnS95 MarijnS95 added this to the Ash 0.38 milestone Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Merge function pointer table struct with high level wrapper struct
2 participants